Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[MRG] construct mgh image only when vox2ras_tkr is needed #241

Merged
merged 4 commits into from
Jul 23, 2019
Merged

[MRG] construct mgh image only when vox2ras_tkr is needed #241

merged 4 commits into from
Jul 23, 2019

Conversation

jasmainak
Copy link
Member

@jasmainak jasmainak commented Jul 20, 2019 •
edited by sappelhoff
Loading

PR Description

closes #238

cc @alexrockhill

Merge checklist

Maintainer, please confirm the following before merging:

  • All comments resolved
  • This is not your own PR
  • All CIs are happy
  • PR title starts with [MRG]
  • whats_new.rst is updated (not updated because write_anat was fixed in the same release cycle)
  • PR description includes phrase "closes <#issue-number>"
  • Commit history does not contain any merge commits

sappelhoff
sappelhoff reviewed Jul 20, 2019
View reviewed changes
Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jasmainak this looks cool. I particularly like how many lines can be saved with the new _validate_bids fixture!

Could it be that the tests fail due to the use of _TempDir? See my similar comment here: #231 (review)

mne_bids/tests/test_write.py Show resolved Hide resolved
mne_bids/tests/test_write.py Outdated Show resolved Hide resolved
mne_bids/tests/test_write.py Show resolved Hide resolved
@jasmainak
Copy link
Member Author

The error seems related to a new commit in the validator: bids-standard/bids-validator@4e6a5b3. Because the tests passed for the older version that I had on my computer. Unfortunately I can't seem to install the latest version for some reason.

@codecov-io
Copy link

Codecov Report

Merging #241 into master will decrease coverage by <.01%.
The diff coverage is 89.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #241      +/-   ##
==========================================
- Coverage   98.26%   98.25%   -0.01%     
==========================================
  Files          15       15              
  Lines        1552     1545       -7     
==========================================
- Hits         1525     1518       -7     
  Misses         27       27
Impacted Files Coverage Δ
mne_bids/write.py 97.06% <100%> (+0.01%) ⬆️
mne_bids/tests/test_write.py 98.79% <87.87%> (-0.04%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a5fccb...0f60a91. Read the comment docs.

@codecov-io
Copy link

codecov-io commented Jul 22, 2019 •
edited
Loading

Codecov Report

Merging #241 into master will decrease coverage by <.01%.
The diff coverage is 89.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #241      +/-   ##
==========================================
- Coverage   98.26%   98.25%   -0.01%     
==========================================
  Files          15       15              
  Lines        1552     1545       -7     
==========================================
- Hits         1525     1518       -7     
  Misses         27       27
Impacted Files Coverage Δ
mne_bids/write.py 97.06% <100%> (+0.01%) ⬆️
mne_bids/tests/test_write.py 98.79% <87.87%> (-0.04%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a5fccb...a14c6a6. Read the comment docs.

sappelhoff
sappelhoff reviewed Jul 22, 2019
View reviewed changes
mne_bids/write.py Outdated Show resolved Hide resolved
@jasmainak
Copy link
Member Author

okay comments addressed @sappelhoff . I added a link. There is some weirdness in the way nibabel treats these header objects -- even if t1w.header['xyzt_units'] == 0 it's not written to disk. I would have imagined it should have the key missing or something. Not sure if it's worth digging more into this. @alexrockhill is our beta tester -- so if he hits another problem, we will fix it as it comes.

@sappelhoff
Copy link
Member

Cool, thanks @jasmainak !

@sappelhoff sappelhoff merged commit d797103 into mne-tools:master Jul 23, 2019
@mghneurotherapeutics
Copy link

The dataset I was testing with did not throw any errors or warnings when I tested it with the updates. Looks great, thanks Mainak!

@sappelhoff sappelhoff mentioned this pull request Sep 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sappelhoff sappelhoff sappelhoff left review comments

@agramfort agramfort agramfort approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Validator warning for write_anat with no acquisition
5 participants
@jasmainak @codecov-io @sappelhoff @mghneurotherapeutics @agramfort